Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Delay start of email inbox #23521

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Conversation

KevLehman
Copy link
Contributor

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

ggazzo
ggazzo previously requested changes Oct 21, 2021
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this combination startup + defer makes sense, if you need some "delay" to solve some race condition or something I'm quite sure we can find a better way

Comment on lines 84 to 86
Meteor.defer(() => {
configureEmailInboxes();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the point doing this? what does this actually fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the issue with this one is that the email inbox depends on a setting being set (Routing_algo). Since the inbox can receive emails before the setting is set (because the setting now runs on a meteor startup) this was aiming to delay the execution of that inbox configuration (there is a task to replace this with a setting watch or another kind of delay too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed there's a watchOnce now for settings, I'll use it and change this to that 👀

@ggazzo
Copy link
Member

ggazzo commented Oct 21, 2021

and please check if you can #22588
I theory when this feature was developed the changes on the classes just broke direct reply(on team collaboration) making the feature unusable

@KevLehman
Copy link
Contributor Author

Oh, I didn't know this feature affected the other (didn't know the other existed tho)

I'll take a look at it later 👀

@KevLehman KevLehman requested a review from ggazzo October 21, 2021 02:01
@sampaiodiego sampaiodiego merged commit efb8cea into develop Oct 21, 2021
@sampaiodiego sampaiodiego deleted the fix/email-inbox-delayed-start branch October 21, 2021 02:13
@sampaiodiego sampaiodiego mentioned this pull request Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants